Proposal for new editors system #1166

saskliutas · 2024-12-20T13:30:22Z

Proposal for new editor system

Proposal for the new editors system. This would replace existing editors system in order to simplify requirements for custom editors. Also it removes static EditorManager in favor of editors registry controlled through React context.

API overview:

EditorsRegistry - Application should use it to register their on custom editors or custom editors provided by some dependencies so that they would be accessible to all components in the application that uses editors system:
- EditorsRegistryProvider - adds supplied editors to the registry hold in React context. It supports nesting multiple EditorsRegistryProvider to allow registering custom editors specific for some component that have higher priority than the ones registered near the root of the application.
- useEditor - hook to get the editor that should be used to edit the supplied value. First it looks for applicable editor in EditorsRegistry and if none was found it fallbacks to the default editors.
Editor (name subject to change) - wrapper around EditorRegistry that provides a convenient way to render editors for specific value.
CommitingEditor (name subject to change) - wrapper around Editor that works as uncontrolled component that commits value on Enter or when editor completes value change (e.g. Toggle click). Also allows to cancel editing on Esc.
Value - type for all values that are supported by editors.
ValueMetadata - type for additional metadata that can be supplied to editors alongside value itself. It can be extended when implementing custom editors. (E.g. passing available choices and icons to the enum editor that is rendered as button group)
Default editors and accompanying use<EditorName>Props hooks that acts as a type guard to conveniently convert from general EditorProps to the specific editor props.
EditorInterop - internal implementation that maps PropertyRecord to the new Value and ValueMetadata types. It is needed to support existing usage on the editors.

Units supports

New editors system was aimed to provide better support for units. There is base component that should help with that FormattedNumericInput. It should be easy to write an editor on top of it that would know how to find Parser/Formatter for specific unit. E.g: https://github.com/iTwin/appui/tree/editors/new-system/ui/imodel-components-react/src/imodel-components-react/inputs/newEditors

Custom editors

The goal of the new editors system is to remove the need for static editor registration and provide more convenient API for implementing custom editors. Current API has quite a lot optional properties that do not make sense (propertyRecord is optional but if it is undefined there is no way to figure out what to render):

Example of custom editor using old editor system and react class components:

interface CustomBooleanEditorState {
  currentValue: boolean;
}

class CustomBooleanEditor
  extends React.PureComponent<PropertyEditorProps, CustomBooleanEditorState>
  implements TypeEditor
{
  private _inputElement = React.createRef<HTMLInputElement>();
  public override readonly state: Readonly<CustomBooleanEditorState> = {
    currentValue: false,
  };

  public async getPropertyValue(): Promise<PropertyValue | undefined> {
    // this is an optional prop for some reason.
    const record = this.props.propertyRecord;
    let propertyValue: PropertyValue | undefined;

    if (record && record.value.valueFormat === PropertyValueFormat.Primitive) {
      propertyValue = {
        valueFormat: PropertyValueFormat.Primitive,
        value: this.state.currentValue,
        displayValue: "",
      };
    }

    return propertyValue;
  }

  public get htmlElement(): HTMLElement | null {
    return this._inputElement.current;
  }

  public get hasFocus(): boolean {
    return document.activeElement === this._inputElement.current;
  }

  public override componentDidUpdate() {
    const { propertyRecord } = this.props;
    if (
      propertyRecord &&
      propertyRecord.value.valueFormat === PropertyValueFormat.Primitive
    ) {
      this.setState({ currentValue: propertyRecord.value.value as boolean });
    }
  this.setState({ currentValue: false });
  }

  public override render() {
    return (
      <ToggleSwitch
        ref={this._inputElement}
        checked={this.state.currentValue}
        onChange={(e) => {
          const newValue = e.currentTarget.checked;
          this.setState({ currentValue: newValue }, () => {
            if (!this.props.propertyRecord || !this.props.onCommit) return;
            this.props.onCommit({
              propertyRecord: this.props.propertyRecord,
              newValue: {
                valueFormat: PropertyValueFormat.Primitive,
                value: newValue,
                displayValue: "",
              },
            });
          });
        }}
      />
    );
  }
}

class CustomBooleanPropertyEditor extends PropertyEditorBase {
  public get reactNode(): React.ReactNode {
    return <CustomBooleanEditor />;
  }
}

Custom editor using old system and react functional components:

const CustomBooleanEditor = React.forwardRef<TypeEditor, PropertyEditorProps>(
  (props, ref) => {
    const inputRef = React.useRef<HTMLInputElement>(null);
    const getCurrentValue = () => {
      if (
        props.propertyRecord &&
        props.propertyRecord.value.valueFormat === PropertyValueFormat.Primitive
      ) {
        return props.propertyRecord.value.value as boolean;
      }
      return false;
    };
    const currentValue = getCurrentValue();

    React.useImperativeHandle(
      ref,
      () => ({
        getPropertyValue: async () => {
          let propertyValue: PropertyValue | undefined;
          if (
            props.propertyRecord &&
            props.propertyRecord.value.valueFormat ===
              PropertyValueFormat.Primitive
          ) {
            propertyValue = {
              valueFormat: PropertyValueFormat.Primitive,
              value: currentValue,
              displayValue: "",
            };
          }
          return propertyValue;
        },
        htmlElement: inputRef.current,
        hasFocus: document.activeElement === inputRef.current,
      }),
      [currentValue, props.propertyRecord]
    );

    return (
      <ToggleSwitch
        ref={inputRef}
        checked={currentValue}
        onChange={(e) => {
          if (!props.propertyRecord || !props.onCommit) return;
          props.onCommit({
            propertyRecord: props.propertyRecord,
            newValue: {
              valueFormat: PropertyValueFormat.Primitive,
              value: e.target.checked,
              displayValue: "",
            },
          });
        }}
      />
    );
  }
);

export class CustomBooleanPropertyEditor extends PropertyEditorBase {
  public get reactNode(): React.ReactNode {
    return <CustomBooleanEditor />;
  }
}

Custom boolean editor using new system:

function getBooleanValue(value: Value | undefined): BooleanValue {
  return value && isBooleanValue(value) ? value : { value: false };
}

export function CustomBooleanEditor(props: EditorProps) {
  // converts passed value into the boolean.
  const currentValue = getBooleanValue(props.value);

  const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
    const newValue = { value: e.target.checked };
    props.onChange(newValue);
    props.onFinish();
  };

  return <ToggleSwitch checked={currentValue.value} onChange={handleChange} />;
}

The new system removes all the code that was associated with class components and accessing values through editor ref. It is not clear if that was used/useful so the chosen approach is to add something similar later if that is still needed. Majority of that was used by EditorContainer that is represented by CommitingEditor in the new system.

TODO

Rewrite existing editors that support PropertyEditorParams from PropertyRecord. Need to find a way how to sunset those PropertyEditorParams in the future but in mean time if should be possible to maintain what is already there in the old system.
Need more work on webfont icons references by Tools in PropertyEditorParams. The initial approach is to maintains internal registry (iconName: string) => ReactNode that would hold currently used icons. Open for suggestions on this one.
Investigate more if current approach can be easily with unit format overrides.
Do we need CommitingEditor as general solutions for committing entered values only on Enter/Blur or each components should have it's own logic to handle such workflows?
Add visual tests.
Add unit tests.
Deprecate old editors.
Remove Presentation from test-app (used for debugging and testing editors feature parity)

GerardasB

Looks good for the most part, however I'd really like to reduce the API surface to a minimum before merging this in (instead of thinking about all the what-if use-cases).
One more major thing to address is the addition of core-quantity peer dep.

I like the iconName -> React.ReactNode mapping for abstract iconSpec types. However, let's do this in a separate PR.

GerardasB · 2024-12-27T09:25:39Z

apps/test-app/src/routes/iTwin_.$iTwinId.tsx

-import { useAuth } from "../frontend/Auth";
-import { PageLayout } from "@itwin/itwinui-layouts-react";
-import { SignInPage } from "../frontend/SignInPage";
+import React from 'react'


Redundant formatter changes

GerardasB · 2024-12-27T09:26:02Z

apps/test-app/src/routes/local_.$fileName.tsx

-import { appInitializer } from "../frontend/AppInitializer";
-import { App } from "../frontend/App";
-import { UiFramework } from "@itwin/appui-react";
+import React from 'react'


Redundant formatter changes

GerardasB · 2024-12-27T09:27:09Z

apps/test-providers/src/tools/SampleTool.ts

      editorPosition: { rowPriority: 21, columnIndex: 2 },
      isDisabled: readonly,
    });
+    const angleDescription = new AngleDescription();


Spreading an instance seems more dangerous (potentially due to getters?).

GerardasB · 2024-12-27T09:29:43Z

apps/test-app/src/frontend/appui/frontstages/EditorFrontstage.tsx

@@ -53,24 +49,6 @@ export function createEditorFrontstageProvider(): UiItemsProvider {
        icon: <SvgEdit />,
      }),
    ],
-    getToolbarItems: () => {


Why was this removed?

GerardasB · 2024-12-27T09:34:03Z

apps/test-app/package.json

@@ -102,7 +102,10 @@
    "@itwin/itwinui-icons-react": "^2.8.0",
    "@itwin/itwinui-layouts-react": "~0.4.1",
    "@itwin/itwinui-layouts-css": "~0.4.0",
+    "@itwin/presentation-backend": "4.4.0",


Are these presentation dependencies necessary to showcase the new editors? Should we simply create a new frontstage for editors and use tool settings/dummy data in one of widgets?
Need to think about names, current EditorFrontstage is really for opening iModels in read-write.
(I see the comment now, that this is actually planned to be removed)

GerardasB · 2024-12-27T09:53:01Z

ui/components-react/src/components-react.ts

@@ -92,6 +92,8 @@ export {
 } from "./components-react/datepicker/DatePickerPopupButton.js";
 export { IntlFormatter } from "./components-react/datepicker/IntlFormatter.js";

+export { EditorInterop } from "./components-react/newEditors/interop/EditorInterop.js";


Should not be exported here if tagged as @internal.

GerardasB · 2024-12-27T09:57:45Z

ui/components-react/src/components-react.ts

@@ -92,6 +92,8 @@ export {
 } from "./components-react/datepicker/DatePickerPopupButton.js";
 export { IntlFormatter } from "./components-react/datepicker/IntlFormatter.js";

+export { EditorInterop } from "./components-react/newEditors/interop/EditorInterop.js";
+export * from "./components-react/newEditors/index.js";


Let's avoid the nested barrel files and especially star re-exports (due to a higher chance of unexpected re-export). Simply inline what's exported here explicitly.
Additionally, I think we should start with only exporting editor components (i.e. useBooleanEditorProps should be marked as @internal).

GerardasB · 2024-12-27T10:04:17Z

ui/components-react/src/components-react/newEditors/editorsRegistry/EditorsRegistryContext.ts

+export const editorsRegistryContext = React.createContext<EditorsRegistry>({
+  editors: [],
+});


Suggested change

export const editorsRegistryContext = React.createContext<EditorsRegistry>({

editors: [],

});

export const EditorsRegistryContext = React.createContext<EditorsRegistry>({

editors: [],

});

EditorsRegistryContext.displayName = "uifw:EditorsRegistryContext";

GerardasB · 2024-12-27T10:08:11Z

ui/components-react/src/components-react/newEditors/CommittingEditor.tsx

+import type { Value } from "./values/Values.js";
+import type { EditorProps } from "./Types.js";
+
+type CommittingEditorProps = Omit<EditorProps, "onFinish" | "onChange"> & {


This means, that CommitingEditor is not a "real editor" and can not be provided via EditorsRegistryProvider?

Nvm, I see this is only adding some keyboard handlers for the regulard Editor.

GerardasB · 2024-12-27T10:08:37Z

ui/components-react/src/components-react/newEditors/Types.ts

+/**
+ * A type that makes a specific properties required in a type.
+ * @beta
+ */
+export type RequiredProps<TProps, TKey extends keyof TProps> = Omit<


Suggested change

/**

* A type that makes a specific properties required in a type.

* @beta

*/

export type RequiredProps<TProps, TKey extends keyof TProps> = Omit<

/**

* A type that makes a specific properties required in a type.

* @internal

*/

export type RequiredProps<TProps, TKey extends keyof TProps> = Omit<

GerardasB · 2024-12-27T10:24:39Z

ui/components-react/src/components-react/newEditors/Types.ts

+  onChange: (value: TValue) => void;
+  onFinish: () => void;
+  disabled?: boolean;
+  size?: "small" | "large";


size should probably not be controlled per editor?

grigasp

Reviewed most of the stuff in components-react and imodel-components-react.

grigasp · 2025-01-09T11:18:29Z

ui/components-react/src/components-react/newEditors/Types.ts

+  metadata: ValueMetadata;
+  value?: TValue;
+  onChange: (value: TValue) => void;
+  onFinish: () => void;


Not clear what onFinish does without looking at the code. Maybe it should be renamed? Or, at least it needs clear docs.

grigasp · 2025-01-10T06:18:44Z

ui/components-react/src/components-react/newEditors/values/Values.ts

+ * @beta
+ */
+export interface InstanceKeyValue {
+  key: { id: string; className: string };


I think using Id64String over string makes it more clear what we expect there.

Suggested change

key: { id: string; className: string };

key: { id: Id64String; className: string };

grigasp · 2025-01-10T06:20:23Z

ui/components-react/src/components-react/newEditors/values/Values.ts

+ * Type guard for text value.
+ * @beta
+ */
+export function isTextValue(


maybe it makes sense to put these type guards under Value namespace?

grigasp · 2025-01-10T06:21:32Z

ui/components-react/src/components-react/newEditors/values/Metadata.ts

+  | "instanceKey";
+
+/**
+ * Additional metadata that is used along side value to determine applicable editor.


do we want to mention "editor" in docs of these types? I could see them being used in non-editing contexts, e.g. read-only property rendering use cases

grigasp · 2025-01-10T06:23:51Z

ui/components-react/src/components-react/newEditors/values/Values.ts

+ */
+export interface EnumValue {
+  choice: number | string;
+  label: string;


does enum value need to have a label? I think it could be retrieved from metadata based on choice. Also, maybe choice should be renamed to value?

grigasp · 2025-01-10T06:34:34Z

ui/components-react/src/components-react/newEditors/values/Metadata.ts

+ * Additional metadata that is used along side value to determine applicable editor.
+ * @beta
+ */
+export interface ValueMetadata {


maybe this should also have information if the value is nullable? in our case most properties will be nullable, but some won't (e.g. some navigation properties like BisCore:Element.Model)

grigasp · 2025-01-10T06:36:47Z

ui/components-react/src/components-react/newEditors/editors/enum/UseEnumEditorProps.ts

+  const defaultValue =
+    choices.length > 0
+      ? { choice: choices[0].value, label: choices[0].label }
+      : { choice: 0, label: "" };


maybe in this case the choice should be empty string instead of 0?

grigasp · 2025-01-10T06:38:23Z

ui/components-react/src/components-react/newEditors/editors/text/UseTextEditorProps.ts

+import { isTextValue } from "../../values/Values.js";
+
+/**
+ * Hooks that converts generic `EditorProps` into editor props with text value. If value is not text, it will be converted into empty text value.


If value is not text, it will be converted into empty text value.

Maybe we should attempt to convert the value to its text representation instead?

grigasp · 2025-01-10T06:42:32Z

ui/components-react/src/components-react/newEditors/editors/numeric/FormattedNumericInput.tsx

+        displayValue:
+          initialValue !== undefined ? formatValue(initialValue) : "",
+      },
+      placeholder: formatValue(123.45),


maybe use initialValue as the placeholder, if it's provided?

grigasp · 2025-01-10T06:52:48Z

ui/imodel-components-react/src/imodel-components-react/editors/NewColorEditor.tsx

+ * @beta
+ */
+export interface ColorValueMetadata extends ValueMetadata {
+  params: ColorEditorParams[];


why is this an array?

saskliutas added 11 commits December 20, 2024 10:52

Initial editor system

13214d2

Use new editors in property grid

2fc52eb

Use new editors in tools

cdb8778

Use new editors in imodel-components-react

9aa93d4

Use new editors in test-app

dfa1a8b

Move formatting stuff to imodel-components-react

910c13f

Cleanup

7201c83

Fix types

22c8d3b

Implement EnumGroupEditor that is compatible with current tools

c52b778

Document core APIs

5fdebac

Cleanup after rebase

12c3105

saskliutas requested review from grigasp and GerardasB December 20, 2024 13:30

No need for base editor props

6a28cab

GerardasB reviewed Dec 27, 2024

View reviewed changes

grigasp reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for new editors system #1166

Proposal for new editors system #1166

saskliutas commented Dec 20, 2024 •

edited

Loading

GerardasB left a comment •

edited

Loading

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024 •

edited

Loading

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024 •

edited

Loading

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024

GerardasB Dec 27, 2024 •

edited

Loading

grigasp left a comment

grigasp Jan 9, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

grigasp Jan 10, 2025

	key: { id: string; className: string };
	key: { id: Id64String; className: string };

Proposal for new editors system #1166

Are you sure you want to change the base?

Proposal for new editors system #1166

Conversation

saskliutas commented Dec 20, 2024 • edited Loading

Proposal for new editor system

Units supports

Custom editors

TODO

GerardasB left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GerardasB Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GerardasB Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GerardasB Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

grigasp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saskliutas commented Dec 20, 2024 •

edited

Loading

GerardasB left a comment •

edited

Loading

GerardasB Dec 27, 2024 •

edited

Loading

GerardasB Dec 27, 2024 •

edited

Loading

GerardasB Dec 27, 2024 •

edited

Loading